Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove aliases resolution limitations when security is enabled #31952

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Jul 11, 2018

Resolving wildcards in aliases expression is challenging as we may end
up with no aliases to replace the original expression with, but if we
replace with an empty array that means _all which is quite the opposite.
Now that we support and serialize the original requested aliases,
whenever aliases are replaced we will be able to know what was
initially requested. MetaData#findAliases can then be updated to not
return anything in case it gets empty aliases, but the original aliases
were not empty. That means that empty aliases are interpreted as _all
only if they were originally requested that way.

Relates to #31516

Resolving wildcards in aliases expression is challenging as we may end
up with no aliases to replace the original expression with, but if we
replace with an empty array that means _all which is quite the opposite.
Now that we support and serialize the original requested aliases,
whenever aliases are replaced we will be able to know what was
initially requested. MetaData#findAliases can then be updated to not
return anything in case it gets empty aliases, but the original aliases
were not empty. That means that empty aliases are interpreted as _all
only if they were originally requested that way.

Relates to elastic#31516
this change is breaking, will not be backported to 6.x
@javanna javanna requested a review from jaymode July 11, 2018 08:48
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@javanna
Copy link
Member Author

javanna commented Jul 11, 2018

I think that I need to add a note to the breaking changes list for xpack, but I am not sure where the list is located.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @lcawl do you know where the x-pack breaking changes for 7.0 get documented?

@@ -32,6 +32,8 @@
*/
String[] aliases();

String[] getOriginalAliases();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add javadocs for this method?

'index/10_with_id/Index with ID',
'indices.get_alias/10_basic/Get alias against closed indices',
'indices.get_alias/20_empty/Check empty aliases when getting all aliases via /_alias',
'indices.get_alias/10_basic/Get alias against closed indices'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 the list gets smaller and smaller

@lcawl
Copy link
Contributor

lcawl commented Jul 11, 2018

The X-Pack-related breaking changes are now integrated in the product-specific documentation (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html)

@javanna
Copy link
Member Author

javanna commented Jul 16, 2018

I am still not sure where I should add this note. @lcawl could you please point me to the file that I need to add this breaking change to? Thanks!

@lcawl
Copy link
Contributor

lcawl commented Jul 16, 2018

@javanna
Copy link
Member Author

javanna commented Jul 17, 2018

thanks @lcawl for the pointers! would you mind double checking the note that I added?


==== Get Aliases API limitations when xpack security is enabled removed

When xpack security is enabled, the Get Aliases API behaviour and response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using "{security}" instead of "xpack security", since the X-Pack terminology is changing in the near future.

==== Get Aliases API limitations when xpack security is enabled removed

When xpack security is enabled, the Get Aliases API behaviour and response
code returned are now aligned to those of vanilla Elasticsearch. Previously
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion "vanilla" might be confusing, since X-Pack is included in Elasticsearch by default now. Maybe something like this might be clearer?: "The behavior and response codes of the get aliases API no longer vary depending on whether {security} is enabled. Previously..."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, that sounds great, thank you.

Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@javanna javanna merged commit 00a6ad0 into elastic:master Jul 20, 2018
martijnvg added a commit that referenced this pull request Jul 21, 2018
* es/master: (23 commits)
  Switch full-cluster-restart to new style Requests (#32140)
  [DOCS] Clarified that you must remove X-Pack plugin when upgrading from pre-6.3. (#32016)
  Remove BouncyCastle dependency from runtime (#32193)
  INGEST: Extend KV Processor (#31789) (#32232)
  INGEST: Make a few Processors callable by Painless (#32170)
  Add region ISO code to GeoIP Ingest plugin (#31669)
  [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236)
  Make sure that field aliases count towards the total fields limit. (#32222)
  Switch rolling restart to new style Requests (#32147)
  muting failing test for internal auto date histogram to avoid failure before fix is merged
  MINOR: Remove unused `IndexDynamicSettings` (#32237)
  Fix multi level nested sort (#32204)
  Enhance Parent circuit breaker error message (#32056)
  [ML] Use default request durability for .ml-state index (#32233)
  Remove indices stats timeout from monitoring docs
  Rename ranking evaluation response section (#32166)
  Dependencies: Upgrade to joda time 2.10 (#32160)
  Remove aliases resolution limitations when security is enabled (#31952)
  Ensure that field aliases cannot be used in multi-fields. (#32219)
  TESTS: Check for Netty resource leaks (#31861)
  ...
if (originalAliases.length > 0 && aliases.length == 0) {
return ImmutableOpenMap.of();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanna I think this is wrong.
If the originalAliases were empty and they got replaced by empty aliases because the user is not authorized for any, then he'll be seeing all of them.
Example:

# create index with 2 random aliases
curl -u elastic-admin:elastic-password -X PUT "localhost:9200/role-index-1" -H "Content-Type: application/json" -d'
{
  "aliases": {
    "some-other-alias": {},
    "some-alias": {}
  }
}
'
# create role that only authorizes on indices and not on any aliases
 curl -u elastic-admin:elastic-password -X POST "localhost:9200/_xpack/security/role/role" -H 'Content-Type: application/json' -d'
{
  "indices": [
    {
      "names": [ "role-index-*" ],
      "privileges": [ "all" ]
    }
  ]
}
'
# assign user to role
curl -u elastic-admin:elastic-password -X POST "localhost:9200/_xpack/security/user/user" -H 'Content-Type: application/json' -d'
{
  "password" : "elastic",
  "roles" : [ "role" ]
}
'
# all aliases are being returned, although only for authorized indices
curl -u user:elastic -X GET "localhost:9200/_alias/*?pretty"
{
  "role-index-1" : {
    "aliases" : {
      "some-other-alias" : { },
      "some-alias" : { }
    }
  }
}
# non-empty aliases list returns an empty response (in the broken way that it does, but it is empty)
curl -u user:elastic -X GET "localhost:9200/_alias/some-*?pretty"
{
  "error" : "alias [some-*] missing",
  "status" : 404
}

I think this should be: if (aliases.length == 0) {
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops, you raise a pretty bad issue. I think that if (aliases.length == 0) { would be too strict, as we would assume that empty always means "none" (the following boolean matchAllAliases = patterns.length == 0; would never be true in that case. The problem is that empty may mean "none" or "all", and it turns out that looking at the original aliases helps in some cases but not always. Maybe we should have some kind of placeholder like we do with indices (Some expression that cannot be used as an alias) to indicate that we should resolve to none, so we can distinguish between these two scenarios.

Copy link
Contributor

@albertzaharovits albertzaharovits Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have some kind of placeholder like we do with indices (Some expression that cannot be used as an alias) to indicate that we should resolve to none, so we can distinguish between these two scenarios.

I think that *, -* should work in 7.0 ?
'-' validation for alias name was added in 3787ea2 (5.1). I am assuming '-' alias names are still existent in 6.x due to bwc but we should migrate the names in 7.0 ? Do we ever enforce name changes for indices if the user ever does rolling upgrades?

Copy link
Contributor

@albertzaharovits albertzaharovits Oct 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel my previous comment requires some clarifications.

I am now certain that in 6.0 you are not allowed to create aliases starting with '-'.
The proof lies underneath:

if (index.charAt(0) == '_' || index.charAt(0) == '-' || index.charAt(0) == '+') {

(A test might though be nice in MetaDataCreateIndexServiceTests, I will see to add one too)

Given that reindex requires first creating the index, and that any index created in < 5.1, when '-' was a valid char in the name, requires reindexing in 6.x before upgrading to 7.x, I think it is safe to assume '-' cannot be part of alias names in 7.x. Therefore *, -* works as a placeholder for no alias in 7.0 .

I will raise the PR soon!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree @albertzaharovits I think this simplifies things, I had not realized that we backported the breaking change to 5.x too. Thanks for checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants